-
Notifications
You must be signed in to change notification settings - Fork 155
ES-2712 #1613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ES-2712 #1613
Conversation
Signed-off-by: ase-101 <[email protected]>
Signed-off-by: ase-101 <[email protected]>
WalkthroughCentralizes server-profile feature flags into a ServerProfile bean, replaces additionalConfig map with explicit boolean flags in OIDCTransaction, adds USE_PKCE and invalid_additional_config error codes, enforces per-transaction PKCE, adjusts JWK validation, and updates tests and OpenAPI enums accordingly. Changes
Sequence Diagram(s)mermaid Client->>AuthService: /authorize (oauth details) request Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@esignet-core/src/main/java/io/mosip/esignet/core/dto/OIDCTransaction.java`:
- Around line 75-90: Update the database seed in the esignet-server profile SQL
so that the fapi2.0 profile no longer sets require_pkce=true: remove or set
require_pkce to false for the fapi2.0 INSERT/UPDATE and keep only PAR
(require_pushed_authorization_requests), DPOP (dpop_bound_access_token), and JWE
settings; then add a new nisdsp profile row in the same seed script with
features require_pushed_authorization_requests=true,
dpop_bound_access_token=true, require_pkce=true and set the proper
userinfo_response_type value (and any related DPoP nonce TTL/JKT columns if
present) so that nisdsp contains PAR, DPoP, PKCE and the userinfo_response_type
as specified.
In `@esignet-service/src/main/java/io/mosip/esignet/config/AppConfig.java`:
- Around line 85-105: Handle a null serverProfile explicitly and replace the
hard-coded error string with a constant: in the serverProfile() method check if
(serverProfile == null || "none".equalsIgnoreCase(serverProfile)) then return
the empty profile to avoid passing null into
serverProfileRepository.findByProfileName; when throwing the exception use
ErrorConstants.INVALID_SERVER_PROFILE instead of the literal
"INVALID_SERVER_PROFILE"; and add a public static final String
INVALID_SERVER_PROFILE = "invalid_server_profile" to the ErrorConstants class so
the constant is defined.
In
`@oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java`:
- Around line 630-635: The Javadoc for the method in AuthorizationServiceImpl
describing defaults is inaccurate: the code actually defaults feature booleans
to false and response type to "JWS" rather than "all available features". Update
the Javadoc to state the actual default behavior (booleans default to false and
response type defaults to "JWS"), or if the intended behavior is to enable all
features by default, change the implementation in the method that sets
additional config on the OIDCTransaction to initialize those booleans to true
and response type accordingly; reference the AuthorizationServiceImpl method
that takes OIDCTransaction and adjusts server/client additional config and the
OIDCTransaction fields that store the booleans and response type to make the
change.
In
`@oidc-service-impl/src/test/java/io/mosip/esignet/services/AuthorizationServiceTest.java`:
- Around line 115-117: Tests are failing because the `@Mock` ServerProfile returns
null for getFeatureMap(), causing an NPE when setFeatureFlags is exercised; in
the AuthorizationServiceTest setUp() method stub ServerProfile#getFeatureMap()
to return an empty Map (e.g., Collections.emptyMap()) so tests have a safe
default; update the setup in AuthorizationServiceTest (the setUp() method) to
add a when(serverProfile.getFeatureMap()).thenReturn(emptyMap) stub before
invoking code that calls setFeatureFlags.
🧹 Nitpick comments (3)
esignet-core/src/main/java/io/mosip/esignet/core/dto/ServerProfile.java (1)
12-17: Consider initializingfeatureMapto prevent potential NPE.While the current usage in
AppConfig.serverProfile()always sets the map, initializing the field defensively would prevent accidental NPE if the DTO is used elsewhere without explicit initialization.♻️ Optional defensive initialization
`@Data` public class ServerProfile { private String name; - private Map<String, String> featureMap; + private Map<String, String> featureMap = new HashMap<>(); }esignet-service/src/test/java/io/mosip/esignet/AppConfigTest.java (2)
34-35: Remove unused@Valueannotation.The
@Valueannotation has no effect in a@ExtendWith(MockitoExtension.class)test since Spring's property injection is not active. The field is never used as all tests set the value viaReflectionTestUtils.♻️ Remove dead code
- `@Value`("${mosip.esignet.server.profile:none}") - private String serverProfile; - `@Test` void serverProfile_ServerProfileIsNone_returnsWithNoFeatures() throws EsignetException {
57-63: Test relies on implicit repository behavior for null input.The test
serverProfile_NullProfile_ThrowsExceptionpasses becausefindByProfileName(null)returnsnullor empty list. However, this test doesn't mock the repository behavior, making the assertion dependent on default mock behavior (returningnull). Consider explicitly mocking for clarity.♻️ Add explicit mock for clarity
`@Test` void serverProfile_NullProfile_ThrowsException() { ReflectionTestUtils.setField(appConfig, "serverProfile", null); + when(serverProfileRepository.findByProfileName(null)).thenReturn(null); EsignetException exception = assertThrows(EsignetException.class, () -> appConfig.serverProfile()); assertEquals("INVALID_SERVER_PROFILE", exception.getMessage()); }
esignet-core/src/main/java/io/mosip/esignet/core/dto/OIDCTransaction.java
Show resolved
Hide resolved
oidc-service-impl/src/main/java/io/mosip/esignet/services/AuthorizationServiceImpl.java
Show resolved
Hide resolved
oidc-service-impl/src/test/java/io/mosip/esignet/services/AuthorizationServiceTest.java
Show resolved
Hide resolved
Signed-off-by: ase-101 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java (1)
226-242: NullPointerException whenalgoruseis missing from JWK.The condition at line 226 now only checks
keyType == null, but line 240 callsalg.isEmpty()anduse.isEmpty(). If a JWK is provided without these fields, this will throw an NPE instead of the expectedEsignetException.🐛 Proposed fix to guard against null values
- if (keyType == null) { + if (keyType == null || alg == null || use == null) { throw new EsignetException(ErrorConstants.INVALID_PUBLIC_KEY); }Alternatively, if null
alg/useshould be allowed to pass the initial check but still validated later:- if (alg.isEmpty() || use.isEmpty()) { + if (alg == null || alg.isEmpty() || use == null || use.isEmpty()) { throw new EsignetException(ErrorConstants.INVALID_PUBLIC_KEY); }
zesu22
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Good
Signed-off-by: ase-101 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@esignet-core/src/main/java/io/mosip/esignet/core/util/IdentityProviderUtil.java`:
- Around line 226-239: The validation in IdentityProviderUtil that checks alg
and use currently only rejects blank values but allows nulls, contradicting
tests; update the validation (in the method that builds the JWK string where
keyType is switched to RsaJsonWebKey/EllipticCurveJsonWebKey) to throw
EsignetException(ErrorConstants.INVALID_PUBLIC_KEY) if alg is null or blank OR
if use is null or blank (i.e., replace the current (alg != null &&
alg.isBlank()) || (use != null && use.isBlank()) check with a null-or-blank
check for both alg and use so missing/null values fail).
🧹 Nitpick comments (3)
esignet-core/src/test/java/io/mosip/esignet/core/IdentityProviderUtilTest.java (3)
231-241: Test name no longer reflects what is being tested.The test
getJWKString_withInvalidAlgForRSA_thenFailnow tests an empty string foralg, not an invalid algorithm for RSA keys. Consider renaming togetJWKString_withEmptyAlg_thenFailfor clarity.Suggested rename
- public void getJWKString_withInvalidAlgForRSA_thenFail() { + public void getJWKString_withEmptyAlg_thenFail() {
243-254: Test name no longer reflects what is being tested.The test
getJWKString_withInvalidUse_thenFailnow tests a blank string foruse, not an invalid use value. Consider renaming togetJWKString_withBlankUse_thenFailfor clarity.Suggested rename
- public void getJWKString_withInvalidUse_thenFail() { + public void getJWKString_withBlankUse_thenFail() {
174-192: Missing test coverage for the new behavior allowing nullalg/use.The existing tests
getJWKString_withMissingAlg_thenFailandgetJWKString_withMissingUse_thenFailpass due to missing RSA key components (n,e), not because of missingalg/use. With the implementation change now allowing nullalganduse, consider adding positive test cases that verify a complete RSA JWK withoutalgorusesucceeds.Suggested additional test
`@Test` public void getJWKString_withValidRSAKeyWithoutAlgAndUse_thenPass() { Map<String, Object> jwkMap = new HashMap<>(); jwkMap.put("kty", "RSA"); jwkMap.put("n", "oahUIzUup5kqncCkHk5Zb1pRrLx7e6YtM-9jX1f5e6mHnZFkC2LJUZ0sEh0n5Y5KnQfW9s7d7gK2b8P0EEl0h3ZyHkWzA3YbsgzB4pDxP4RxMZ1I8xD2z3UvfA1zjvKDHz6wEweq4hVJ8nS8GzZJ2E_vb3s"); jwkMap.put("e", "AQAB"); // No alg or use - should now pass String jwkJson = IdentityProviderUtil.getJWKString(jwkMap); Assertions.assertTrue(jwkJson.contains("\"kty\":\"RSA\"")); }
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.